feat(contrib): add PR review and module scaffold skills#131
Conversation
Adds two Claude Code skills to .claude/skills/ for SDK contributors: - review-pr: reviews a PR against 23 criteria across 6 sections (process, security, code quality, API design, tests, documentation). Runs from the repo root via gh CLI. Outputs a structured table report with verdict and optional gh pr review posting. - scaffold-module: generates the full standard module layout (10 files + telemetry wiring) for a new BTP service integration. Includes a self-review phase that runs ruff and fixes issues before reporting. Both skills are compatible with Claude Code, Cline, Copilot, and other agentic tools via the tools/compatibility frontmatter fields.
Replace diff-offset line counting with explicit file fetches. Phase 2 now downloads each changed file to /tmp/pr<number>/<path> at the PR head ref; Phase 3 reads those files with the Read tool for exact line numbers instead of deriving positions from unified diff hunk arithmetic.
| ## Installation | ||
|
|
||
| ```bash | ||
| uv add sap-cloud-sdk --index-url "https://int.repositories.cloud.sap/artifactory/api/pypi/proxy-3rd-party-pypi/simple" |
There was a problem hiding this comment.
The generated user-guide.md includes an SAP-internal Artifactory URL. This public repo’s PR template explicitly disallows SAP-internal URLs, could be the new review-pr skill flag this as sensitive data?
There was a problem hiding this comment.
Fixed in 82fca05 — replaced the internal Artifactory URL with plain uv add sap-cloud-sdk.
| url=self.url, | ||
| client_id=self.clientid, | ||
| client_secret=self.clientsecret, | ||
| token_url=self.url.rstrip("/") + "/oauth/token", |
There was a problem hiding this comment.
Deriving token_url from url + "/oauth/token" may be wrong for many BTP bindings (service URL vs UAA/auth URL are often separate, e.g. uri + url like in destination).
Consider asking for the binding schema during scaffold input, could be leaving an explicit TODO instead of a concrete default that could mislead new modules?
There was a problem hiding this comment.
Good catch. Fixed in 82fca05 — removed the derived token_url and replaced it with a TODO comment explaining that BTP binding schemas vary and the UAA URL must come from the binding itself, not be guessed from the service URL.
| Run lint to verify the generated files are clean: | ||
| ```bash | ||
| uv run ruff check src/sap_cloud_sdk/<MODULE_NAME>/ | ||
| ``` |
There was a problem hiding this comment.
Self-review only runs ruff check on src/. docs/DEVELOPMENT.md recommends the full local gate: ruff check ., ruff format --check ., and ty check ..
Please align the skill so “clean baseline” includes format + type check and I believe that ideally pytest on the generated tests, correct if I`m wrong!
There was a problem hiding this comment.
All good points, @tiagoek .
Thanks for the review, I'll add your suggestions.
There was a problem hiding this comment.
Fixed in 82fca05 — Phase 5 self-review now runs the full gate: ruff check, ruff format --check, ty check, and pytest.
Phase 1 now parses REPO and NUMBER from three input forms: full GitHub URL, owner/repo#number, or plain number (defaults to SAP/cloud-sdk-python). All gh commands and the file-fetch API call use the resolved REPO so the skill works for PRs in forks, not just PRs in the upstream repo. Also switch file-fetch ref from branch name to head commit SHA to avoid ambiguity when fork and upstream share the same branch name.
- Remove SAP-internal Artifactory URL from user-guide.md template; replace with plain `uv add sap-cloud-sdk` - Remove incorrect token_url derivation from service URL; BTP binding schemas vary and the UAA URL must be read from the binding, not guessed from the service URL - Expand Phase 5 self-review to run the full local gate: ruff check, ruff format --check, ty check, and pytest (not just ruff check)
3 participants
Description
Adds two Claude Code skills to `.claude/skills/` for SDK contributors. Skills are invocable via Claude Code CLI (or any compatible agentic tool: Cline, Copilot, etc.) from the repo root.
`/review-pr [PR#]`
Reviews a PR against 23 criteria across 6 sections:
Outputs a structured table report with ✅/⚠️ /❌/➖ per criterion, blocking issues summary, and optional posting via `gh pr review`.
Line citations are accurate: Phase 2 fetches each changed file at the PR head commit SHA via `gh api`, so all `file:line` references in the report come from `Read` output on the actual files rather than from diff hunk arithmetic.
Accepts a plain PR number (defaults to SAP/cloud-sdk-python), a full GitHub URL, or `owner/repo#number` — so it works for PRs in forks as well as the upstream repo.
`/scaffold-module`
Generates the full standard module layout for a new BTP service integration:
Type of Change
How to Test
Checklist